Skip to content

[v22.x backport] src: initialize privateSymbols for per_context #58217

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: v22.x-staging
Choose a base branch
from

Conversation

jazelly
Copy link
Member

@jazelly jazelly commented May 7, 2025

Backport 2 commits where the second one was rebased on the first one.

src: ensure primordials are initialized exactly once
src: initialize privateSymbols for per_context

The manual backport bypassed the usage of Get/SetPrototypeV2 that does not exist on v22.x.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/realm
  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v22.x v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. labels May 7, 2025
@jazelly jazelly added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label May 7, 2025
@jazelly jazelly closed this May 10, 2025
@legendecas
Copy link
Member

@jazelly Any reason why this was closed? Would you like to pick it up again?

@jazelly
Copy link
Member Author

jazelly commented May 13, 2025

I got a couple of JS crash/OOM in GHA like https://github.com/nodejs/node/actions/runs/14944261967/job/41985588150?pr=58217

I was thinking maybe this cannot be backported without backporting #55453? Was closing this to give me more time to debug this.

@legendecas
Copy link
Member

I think these crashes are irrelevant to this change.

@jazelly jazelly reopened this May 13, 2025
@jazelly
Copy link
Member Author

jazelly commented May 13, 2025

For this GHA failed build on windows

D:\a\node\node\deps\v8\include\v8-internal.h(1159,36): error C2607: static assertion failed [D:\a\node\node\libnode.vcxproj]
  (compiling source file '/src/node_messaging.cc')
  
D:\a\node\node\deps\v8\include\v8-internal.h(1180,37): error C7683: you cannot create a reference to 'void' [D:\a\node\node\libnode.vcxproj]
  (compiling source file '/src/node_messaging.cc')

I got it reproduced in my windows machine, and reverting #57578 fixed it.

It failed due to a major change that lives in v23 and after but not before, specifically this one.

The LocalVector was introduced before/in v22, but it was then stablized in v23 with that change. I don't think #57578 should be back ported to v22.

Not sure if we have v22.x-staging build periodically. cause I believe the v22.x staging won't build on windows successfully with that LocalVector change.

@richardlau
Copy link
Member

Not sure if we have v22.x-staging build periodically. cause I believe the v22.x staging won't build on windows successfully with that LocalVector change.

We have daily builds of v22.x-staging https://ci.nodejs.org/view/Node.js%20Daily/job/node-daily-v22.x-staging/
(Link won't work right now for most people as the Jenkins CI is restricted at the moment while we're preparing this week's security release.)

It looks like Windows builds have been failing since 7 May with e0a025a -- they were passing the day before with 3f5899f.

@jazelly
Copy link
Member Author

jazelly commented May 13, 2025

I got it reproduced in my windows machine, and reverting #57578 fixed it.

Sorry I put a wrong link before. I was talking about this back port commit from #57733

it looks like Windows builds have been failing since 7 May with e0a025a

#57733 was back ported to v22.x-staging 5 hours before that, so it could be the one that causing the failed build.

If the failure log on windows about the the static assertion, then I think we need to revert that back port.

D:\a\node\node\deps\v8\include\v8-internal.h(1159,36): error C2607: static assertion failed [D:\a\node\node\libnode.vcxproj]
  (compiling source file '/src/node_messaging.cc')
  
D:\a\node\node\deps\v8\include\v8-internal.h(1180,37): error C7683: you cannot create a reference to 'void' [D:\a\node\node\libnode.vcxproj]
  (compiling source file '/src/node_messaging.cc')

I can raise a separate revert PR to revert backport c408a7f. But I am not sure about the OOM on linux, which I guess are also failing the v22.x-staging build.

@RafaelGSS RafaelGSS requested a review from a team as a code owner May 14, 2025 20:50
@aduh95
Copy link
Contributor

aduh95 commented May 16, 2025

@jazelly thanks for investigating that, I'm able to confirm that c408a7f is the culprit:
https://ci.nodejs.org/job/node-test-commit-windows-fanned/70430/ passes and https://ci.nodejs.org/job/node-test-commit-windows-fanned/70436/ doesn't.

I've removed it from the staging branch

@jazelly jazelly force-pushed the backport-57479 branch 2 times, most recently from e611888 to 1e5c8a2 Compare May 17, 2025 01:00
@aduh95 aduh95 force-pushed the v22.x-staging branch 3 times, most recently from e0a025a to d03f13a Compare May 17, 2025 13:54
@aduh95 aduh95 changed the title [v22.x backport] Backport 57519 and 57479 [v22.x backport] src: initialize privateSymbols for per_context May 17, 2025
PR-URL: nodejs#57519
Backport-PR-URL: nodejs#58217
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v22.x v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants